Add feature flag to allow VS to use the LSP based editor #49996

dibarbet · 2020-12-16T00:30:28Z

Adds a feature flag to use the LSP editor for C#/VB in local scenarios. This flag is only shown in the preview features menu in int.main and int.preview.

Additionally, adds test infrastructure for running LSP tests

CyrusNajmabadi · 2020-12-16T21:53:28Z

src/EditorFeatures/Core.Wpf/InlineRename/CommandHandlers/RenameCommandHandler_RenameHandler.cs

@@ -108,7 +109,8 @@ private static bool CanRename(RenameCommandArgs args)
        {
            return args.SubjectBuffer.TryGetWorkspace(out var workspace) &&
                workspace.CanApplyChange(ApplyChangesKind.ChangeDocument) &&
-                args.SubjectBuffer.SupportsRename();
+                args.SubjectBuffer.SupportsRename() &&
+                !workspace.Services.GetService<IWorkspaceContextService>().IsInLspEditorContext();


i dislike this. why is this not part of the .SupportsRename extension? seems weird for the buffer to say it supports this and we then override.

put another way, i highly dislike this sort of sprinkling aruond of a different concept that features should not have to care about.

@CyrusNajmabadi this is ready for review now - I moved this to the supports rename, but in general I agree and don't really like sprinkling this around either, but I'm not sure I have a better idea.

Basically I need to disable the legacy editor entry points when running under the LSP editor for features that are provided by LSP (e.g. rename) but need to keep other editor entry points active (e.g. syntax classifications).

I was potentially considering perhaps modifying the extension methods to convert textbuffer/snapshot to documents (GetOpenDocumentInCurrentContextWithChanges) to return nothing when under LSP mode, but then I'd still need a separate method to allow some features to retrieve documents from the buffer (e.g. syntax classification)

@CyrusNajmabadi I've gotten these new integration tests working again and am looking to get this in, so wanted to followup on this conversation. Can discuss offline whenever you have a meeting free day :)

src/EditorFeatures/Core.Wpf/Suggestions/SuggestedActionsSource.cs

src/EditorFeatures/Core/Implementation/Classification/SemanticClassificationUtilities.cs

CyrusNajmabadi

basicaly, i don't understand what's goign on here. i have no intuition for when/how/why we need to sprinkle this stuff around.

This reverts commit cdad03e.

dibarbet · 2021-01-15T23:35:37Z

eng/build.ps1

@@ -372,6 +373,13 @@ function TestUsingRunTests() {
    $args += " --sequential"
    $args += " --include '\.IntegrationTests'"
    $args += " --include 'Microsoft.CodeAnalysis.Workspaces.MSBuild.UnitTests'"
+
+    if ($lspEditor) {
+      $args += " --testfilter FullyQualifiedName~Roslyn.VisualStudio.IntegrationTests.LanguageServerProtocol|Editor=LanguageServerProtocol"


I went with both a separate namespace and additional trait name to enable multiple testing scenarios:

Running selected existing integration tests under the LSP editor (uses trait to select which tests).

Writing LSP only integration tests (uses separate namespace).

Ensure that the tests in 2) do not run in normal integration test runs.

I guess the trait is sufficient right, if you just put the trait on containing types? My strong worry about the magic namespace is that we might rename or move something and not realize that tests aren't running anymore. Having the single method with the trait makes it a bit easier to know everything either works or doesn't.

(see other comment though that the only test currently in it's own namespace I think is better just combined with the existing test which checks if it's in LSP mode or not...)

And yes, we once had an integration test CI that was passing because whatever glob was looking for assemblies to run wasn't updated, and so it was running nothing. That was a "fun" discovery.

@jasonmalinowski If we ever want to support running a test in LSP only context then we must have a separate namespace or something similar. Otherwise I have no way to filter out LSP-only tests from the normal run (since the normal run includes shared tests with the LSP trait).

I can guarantee that there will be LSP only tests, if not gotodef then completion or another feature. I think even for the gotodef test you mention I must have an LSP version and a non-LSP version, even if I share most of the code as they have different input data (window caption).

I guess as long as we're using environment variables to control this, there's no reason you can just haven't a ConditionalFact that only runs if the environment variable is set. You can also have the test just check the environment variable in a test if you need to change a string.

Ah! Today I learned about conditional facts. Ok, I'll use that in a followup instead.

dibarbet · 2021-01-15T23:38:12Z

src/Tools/Source/RunTests/ITestExecutor.cs

@@ -13,18 +13,16 @@ namespace RunTests
        internal string DotnetFilePath { get; }
        internal ProcDumpInfo? ProcDumpInfo { get; }
        internal string TestResultsDirectory { get; }
-        internal string? Trait { get; }
-        internal string? NoTrait { get; }
+        internal string? TestFilter { get; }


As best as I could tell, the trait and notrait properties did not work properly with dotnet test. They would pass an argument like Trait=LanguageServerProtocol, but per https://docs.microsoft.com/en-us/dotnet/core/testing/selective-unit-tests?pivots=xunit that isn't the correct format. So I modified these to be able to just pass in the xunit test filter (which also allowed me to pass in fully qualified names)

is it possible to extract this out into anotehr pR?

dibarbet · 2021-01-15T23:39:42Z

src/VisualStudio/IntegrationTest/TestUtilities/OutOfProcess/Editor_OutOfProc.cs

+        public void GoToDefinition(string viewName)
+        {
+            _editorInProc.GoToDefinition();
+            _editorInProc.WaitForActiveView(viewName);


LSP goto definition is async so we need to wait until the tab is opened. this shouldn't affect local tests as that tab will be opened immediately.

100ms doesn't seem long enough here. i coudl easily see things failing on CI because the machine is doing just a little bit of work, and can't open the window in time.

JoeRobich

Infrastructure changes look good.

dibarbet · 2021-02-24T19:58:31Z

I think I've addressed the feedback, let me know if there is any more
@CyrusNajmabadi @jasonmalinowski

jasonmalinowski

Leaving comments so far.

eng/pipelines/test-integration-job.yml

src/VisualStudio/Core/Def/Implementation/LanguageClient/LiveShareInProcLanguageClient.cs

src/VisualStudio/Core/Def/PackageRegistration.pkgdef

src/VisualStudio/Setup.Dependencies/Roslyn.VisualStudio.Setup.Dependencies.csproj

src/VisualStudio/IntegrationTest/TestUtilities/VisualStudioInstanceFactory.cs

src/VisualStudio/IntegrationTest/TestUtilities/OutOfProcess/Editor_OutOfProc.cs

src/VisualStudio/IntegrationTest/IntegrationTests/LanguageServerProtocol/LspGoToDefinition.cs

src/VisualStudio/IntegrationTest/TestUtilities/InProcess/Editor_InProc.cs

...ures/Core/Implementation/Classification/SemanticClassificationBufferTaggerProvider.Tagger.cs

src/VisualStudio/Core/Def/Implementation/VisualStudioSupportsFeatureService.cs

jasonmalinowski · 2021-03-02T04:33:20Z

eng/build.ps1

@@ -372,6 +373,13 @@ function TestUsingRunTests() {
    $args += " --sequential"
    $args += " --include '\.IntegrationTests'"
    $args += " --include 'Microsoft.CodeAnalysis.Workspaces.MSBuild.UnitTests'"
+
+    if ($lspEditor) {
+      $args += " --testfilter FullyQualifiedName~Roslyn.VisualStudio.IntegrationTests.LanguageServerProtocol|Editor=LanguageServerProtocol"


I guess the trait is sufficient right, if you just put the trait on containing types? My strong worry about the magic namespace is that we might rename or move something and not realize that tests aren't running anymore. Having the single method with the trait makes it a bit easier to know everything either works or doesn't.

jasonmalinowski · 2021-03-02T04:34:04Z

eng/build.ps1

@@ -372,6 +373,13 @@ function TestUsingRunTests() {
    $args += " --sequential"
    $args += " --include '\.IntegrationTests'"
    $args += " --include 'Microsoft.CodeAnalysis.Workspaces.MSBuild.UnitTests'"
+
+    if ($lspEditor) {
+      $args += " --testfilter FullyQualifiedName~Roslyn.VisualStudio.IntegrationTests.LanguageServerProtocol|Editor=LanguageServerProtocol"


(see other comment though that the only test currently in it's own namespace I think is better just combined with the existing test which checks if it's in LSP mode or not...)

jasonmalinowski · 2021-03-02T04:34:54Z

eng/build.ps1

@@ -372,6 +373,13 @@ function TestUsingRunTests() {
    $args += " --sequential"
    $args += " --include '\.IntegrationTests'"
    $args += " --include 'Microsoft.CodeAnalysis.Workspaces.MSBuild.UnitTests'"
+
+    if ($lspEditor) {
+      $args += " --testfilter FullyQualifiedName~Roslyn.VisualStudio.IntegrationTests.LanguageServerProtocol|Editor=LanguageServerProtocol"


And yes, we once had an integration test CI that was passing because whatever glob was looking for assemblies to run wasn't updated, and so it was running nothing. That was a "fun" discovery.

stale, re-requested

dibarbet · 2021-03-02T18:28:12Z

/azp run

azure-pipelines · 2021-03-02T18:28:34Z

Azure Pipelines successfully started running 3 pipeline(s).

jasonmalinowski · 2021-03-02T19:16:55Z

src/VisualStudio/IntegrationTest/IntegrationTests/CSharp/CSharpGoToDefinition.cs

@@ -69,26 +69,30 @@ public void GoToDefinitionOpensProvisionalTabIfDocumentNotAlreadyOpen()
    SomeClass sc;
 }");
            VisualStudio.Editor.PlaceCaret("SomeClass");
-            VisualStudio.Editor.GoToDefinition();
+            VisualStudio.Editor.GoToDefinition("FileDef.cs");
            VisualStudio.Editor.Verify.TextContains(@"class SomeClass$$", assertCaretPosition: true);
            Assert.True(VisualStudio.Shell.IsActiveTabProvisional());
        }

        [WpfFact, Trait(Traits.Feature, Traits.Features.GoToDefinition)]


So I would have expected xUnit would see this method through the inherited class as well, and then in the inherited copy see this method with LSP trait, so it'd run this in the LSP case as well. If this works it works, but not entirely sure what might happen here.

Dotnet-GitSync-Bot added the Area-IDE label Dec 16, 2020

Add feature flag to allow VS to use the LSP based editor

a4d4605

CyrusNajmabadi reviewed Dec 16, 2020

View reviewed changes

src/EditorFeatures/Core.Wpf/Suggestions/SuggestedActionsSource.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 16, 2020

View reviewed changes

src/EditorFeatures/Core/Implementation/Classification/SemanticClassificationUtilities.cs Outdated Show resolved Hide resolved

CyrusNajmabadi previously requested changes Dec 16, 2020

View reviewed changes

dibarbet force-pushed the local_lsp branch from 1e12a19 to 9249d5a Compare January 5, 2021 06:49

Add integration test support for LSP based editor

97c54a2

dibarbet force-pushed the local_lsp branch from 9249d5a to 97c54a2 Compare January 5, 2021 06:51

dibarbet added 8 commits January 5, 2021 18:06

Add new integration test pipeline

a16ad35

Merge remote-tracking branch 'upstream/master' into local_lsp

2a9b7e4

Fix type specification

f7444f7

Change parameter types to string to allow variables to be passed in

35e35d8

Fix relative reference to pipeline template

c10d1cd

Fix template parameter access

34d355d

Add LSP logs to test run output

2b0f6a6

Merge remote-tracking branch 'upstream/master' into local_lsp

8421485

dibarbet force-pushed the local_lsp branch 2 times, most recently from be6dc7b to 2cda1db Compare January 15, 2021 03:08

Fix tests

ce45c40

dibarbet force-pushed the local_lsp branch from 2cda1db to ce45c40 Compare January 15, 2021 03:14

dibarbet added 3 commits January 15, 2021 14:55

cleanup

1b9d87a

test failing test

cdad03e

Revert "test failing test"

2a27f8a

This reverts commit cdad03e.

dibarbet commented Jan 15, 2021

View reviewed changes

dibarbet marked this pull request as ready for review January 15, 2021 23:45

dibarbet requested review from a team as code owners January 15, 2021 23:45

dibarbet added 6 commits February 23, 2021 15:24

move semantic tokens lsp editor check to tagger impl

7ccdf64

Update capabilities provided in alwaysactive server

d0305d9

remove workaround now that integration test machines are updated

75f8c04

Add documentation on why there are LSPspecific tests

b3288f4

add comment on integration lsp yml

f58231c

move check into code action provider

38179be

dibarbet requested a review from a team as a code owner February 24, 2021 02:57

JoeRobich approved these changes Feb 24, 2021

View reviewed changes

Merge branch 'master' into local_lsp

edf1c86

jasonmalinowski reviewed Mar 2, 2021

View reviewed changes

Feedback

39423e8

dibarbet force-pushed the local_lsp branch from 9519305 to 39423e8 Compare March 2, 2021 05:51

fix test definition

a894943

jasonmalinowski reviewed Mar 2, 2021

View reviewed changes

jasonmalinowski approved these changes Mar 2, 2021

View reviewed changes

dibarbet added 3 commits March 2, 2021 11:31

Fact -> WpfFact

e1fa552

Fix tests to use window name instead of view name

68b9691

Remove accidentally added fact...

bdb3aa5

dibarbet merged commit ae76121 into dotnet:master Mar 3, 2021

ghost added this to the Next milestone Mar 3, 2021

dibarbet deleted the local_lsp branch March 3, 2021 00:29

dibarbet mentioned this pull request Mar 3, 2021

Remove namespace test filter #51632

Merged

allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021

jmarolf mentioned this pull request Feb 7, 2022

Add repo files to Roslyn.sln and Compilers.sln as solution items #59328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature flag to allow VS to use the LSP based editor #49996

Add feature flag to allow VS to use the LSP based editor #49996

dibarbet commented Dec 16, 2020 •

edited

Loading

CyrusNajmabadi Dec 16, 2020

CyrusNajmabadi Dec 16, 2020

dibarbet Jan 15, 2021

dibarbet Feb 19, 2021

CyrusNajmabadi left a comment

dibarbet Jan 15, 2021

jasonmalinowski Mar 2, 2021 •

edited

Loading

jasonmalinowski Mar 2, 2021

jasonmalinowski Mar 2, 2021

dibarbet Mar 2, 2021

jasonmalinowski Mar 2, 2021

dibarbet Mar 2, 2021

dibarbet Jan 15, 2021

CyrusNajmabadi Feb 23, 2021

dibarbet Jan 15, 2021

CyrusNajmabadi Feb 23, 2021

JoeRobich left a comment

dibarbet commented Feb 24, 2021

jasonmalinowski left a comment

jasonmalinowski Mar 2, 2021 •

edited

Loading

jasonmalinowski Mar 2, 2021

jasonmalinowski Mar 2, 2021

dibarbet commented Mar 2, 2021

azure-pipelines bot commented Mar 2, 2021

jasonmalinowski Mar 2, 2021

Add feature flag to allow VS to use the LSP based editor #49996

Add feature flag to allow VS to use the LSP based editor #49996

Conversation

dibarbet commented Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeRobich left a comment

Choose a reason for hiding this comment

dibarbet commented Feb 24, 2021

jasonmalinowski left a comment

Choose a reason for hiding this comment

jasonmalinowski Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibarbet commented Mar 2, 2021

azure-pipelines bot commented Mar 2, 2021

Choose a reason for hiding this comment

dibarbet commented Dec 16, 2020 •

edited

Loading

jasonmalinowski Mar 2, 2021 •

edited

Loading

jasonmalinowski Mar 2, 2021 •

edited

Loading